While technically a BC break, this is necessary for us to allow returning a custom iterator class. The Cursor class implements both interfaces, so the public API does not change. If users explicitly check the returned class type, this may cause unintended behaviour.

This is to support watch operations with a codec supplied, so we can avoid messing with the type map on every iteration.

Passing a Document instance as resume token would be valid, but not converting it to PHP causes test failures. In our tests, we extract the resume token from the command response, which is always in PHP format. To avoid issues when testing watch operations with a codec, this is always converted to a PHP value here as well.

This is a subtle behavioral change because we're now deferring server selection until after constructing the Count operation, but I'll note that other helper methods vary in this regard.

For examples, helpers like aggregate() must select servers in advance to handle option inheritance.

I think it does make sense to defer server selection as long as possible, since it makes it easier to catch other client-side validation errors sooner.

Consider adding a regression test for the issue I mentioned in https://github.com/mongodb/mongo-php-library/pull/1140/files#r1284677901. You'd need some findAndModify options to result in value being unset on the response.

See findAndModify: value for two possible conditions to trigger that response.

Added tests that use the "return null if nothing was matched" logic to ensure we're returning the correct values. Thank you for highlighting this once again as I missed this the first time around.

The Collection instance is created internally, so I don't see how someone could pass on a codec to get a different result. The return type will always be Cursor.

The $options passed to Bucket::find are passed down to Collection::find as-is, so a user could introduce a codec that way, which would result in a return value of a type other than Cursor.

That said, this makes me wonder whether we should directly support a codec option in the Bucket class as well (similar to how typeMap is already supported). I haven't considered that until now, so if we decide to go that route I'd do so in a separate ticket and pull request.

The $options passed to Bucket::find are passed down to Collection::find as-is, so a user could introduce a codec that way, which would result in a return value of a type other than Cursor.

Noted that this explains the CursorInterface&Iterator return type being added here.

That said, this makes me wonder whether we should directly support a codec option in the Bucket class as well (similar to how typeMap is already supported).

The typeMap option is only used for getFileDocumentForStream() and getFileIdForStream(), and uploadFromStream() indirectly since that calls getFileIdForStream().

I think it does make sense to support codec there as we do typeMap today. Please create a separate ticket for that.

Created PHPLIB-1220.

After checking, psalm seems to accept union types here.

Thank you for adding these tests, per the recent change to FindAndModify to directly return null instead of calling decode().

In a previously resolved thread (https://github.com/mongodb/mongo-php-library/pull/1140/files#r1272549069), we talked about why codec is not passed down to Aggregate while typeMap is.

Looking at this once more alongside the changes in ChangeStream to apply the codec upon construction and when resuming, I think there's an edge case where typeMap and codec are both specified as a Watch option. If that happens, I imagine the typeMap will just get ignored when the ChangeStream is constructed and modifies its inner iterator (i.e. the cursor).

We prohibit that scenario for other operations, such as Find:

if (isset($options['codec']) && isset($options['typeMap'])) {
    throw InvalidArgumentException::cannotCombineCodecAndTypeMap();
}

Should we do so later in this constructor and add a test in WatchTest?

Good point. I tested to confirm it works as expected with codec AND typeMap specified. I agree that specifying both options should be prohibited as we do in other operations. Added the exception and a test as suggested.

Watch constructs the ChangeStreamIterator and has access to its internal cursor, even though it currently sends it as-is directly from the private executeAggregate() method. Watch also constructs the $resumeCallable here.

I was going to ask why ChangeStream bothers with calling setTypeMap(['root' => 'bson']) when that could otherwise be done in Watch; however, even if we did move that logic to Watch, ChangeStream would still need to know about the codec in order to process return values for current().

And you previously explained that Watch cannot apply the codec itself since doing so might interfere with resume token collection.

I think this is fine as-is but just want to confirm with you before signing off. Feel free to resolve after reading if my understanding is correct.

Another reason for asking was that ChangeStream is in the public API (although undocumented), so I'm paying extra attention the signature change above.

Since ChangeStream needs to be in control of the codec used in the cursor (as it needs to be sure about the return types), I figured it would make more sense for ChangeStream to apply its codec to the underlying cursor instead of relying on that being set correctly. As is, Watch is only accepting the codec option on behalf of ChangeStream, and passing it on without needing to know about internal workings of ChangeStream.

The main problem with applying the codec in the original Aggregate operation is that while the aggregation pipeline results will always have an _id field to be used as resume token, we cannot make such guarantees after the codec has been applied. After that, the _id field may have been discarded or assigned to an object property we don't know about.

Also note that while ChangeStream and ChangeStreamIterator are part of the public API, both their constructors are marked as internal; indicating that while we expect users to interact with these classes, we don't expect them to instantiate them on their own. The signature change here is backward compatible, as the $codec parameter defaults to null, so any previously working instantiations of that class will continue to function as expected.